feat: enable /logs API endpoint in standalone (Docker CE) mode#824
feat: enable /logs API endpoint in standalone (Docker CE) mode#824ilopezluna wants to merge 2 commits intomainfrom
Conversation
When MODEL_RUNNER_LOG_DIR is not set, auto-create a default log directory (/tmp/model-runner-logs) and set up tee logging so that all output goes to both stderr (for docker logs) and bracket- timestamped log files (for the /logs API endpoint). Changes: - Add BracketWriter to pkg/logging that prefixes each line with [timestamp] in the format expected by MergeLogs - Auto-create log directory and open service + engine log files - Use io.MultiWriter to tee slog output to stderr and log files - Configure ServerLogFactory for engine backend logs - Always enable /logs endpoint when a log directory is available - Fix comments that incorrectly stated Docker Desktop uses MODEL_RUNNER_LOG_DIR (it uses its own log infrastructure)
There was a problem hiding this comment.
Code Review
This pull request enables the /logs API endpoint across all deployment modes by automatically creating a default log directory and implementing file-backed logging using a new BracketWriter. The changes improve operational visibility and debugging capabilities, supported by new unit tests and goroutine leak detection. A critical issue was identified in pkg/logging/logging.go at lines 117-123, where the BracketWriter.Write method risks data loss by clearing its internal buffer before confirming a successful write to the underlying writer; the buffer must only be advanced after the write succeeds.
Move buffer advancement after the successful write to the underlying writer. Previously, the line was removed from the buffer before the write, so a failed write (e.g., full disk) would lose the log entry. Also avoids an unnecessary string allocation by passing the byte slice directly to fmt.Fprintf.
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new LogDir() comment suggests the server auto-creates a default directory, but this behavior actually lives in main; consider updating the comment (or moving the logic) so the function comment accurately reflects its behavior.
- BracketWriter currently buffers until a newline is seen and never flushes partial lines, which can cause unbounded memory growth and dropped logs on process exit; consider adding a Flush/Close path or a max-buffer safeguard.
- Engine logging uses a shared *os.File with separate BracketWriters created per backend; if multiple backends write concurrently, their prefix+line writes to the shared file may interleave—consider either sharing a single BracketWriter per file or serializing access at a higher level.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new LogDir() comment suggests the server auto-creates a default directory, but this behavior actually lives in main; consider updating the comment (or moving the logic) so the function comment accurately reflects its behavior.
- BracketWriter currently buffers until a newline is seen and never flushes partial lines, which can cause unbounded memory growth and dropped logs on process exit; consider adding a Flush/Close path or a max-buffer safeguard.
- Engine logging uses a shared *os.File with separate BracketWriters created per backend; if multiple backends write concurrently, their prefix+line writes to the shared file may interleave—consider either sharing a single BracketWriter per file or serializing access at a higher level.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
No BracketWriter.Flush/Close, missing integration test for auto-created log dir, slightly misleading envconfig comment |
When
MODEL_RUNNER_LOG_DIRis not set, auto-create a default log directory/tmp/model-runner-logsand set up tee logging so that all output goes to both stderr and bracket- timestamped log files (for the /logs API endpoint).Changes: